New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
internal/credentials: fix a bug and add one more helper function SPIFFEIDFromCert #3929
Conversation
@@ -57,7 +67,7 @@ func SPIFFEIDFromState(state tls.ConnectionState) *url.URL { | |||
return nil | |||
} | |||
// A valid SPIFFE certificate can only have exactly one URI SAN field. | |||
if len(state.PeerCertificates[0].URIs) > 1 { | |||
if len(cert.URIs) > 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be checked outside of the for loop? (probably alongside the nil
check)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move it outside, the logging can't be moved together because at that time we are not sure the user is using SPIFFE ID yet. In current implementation, we will only log the warning under the condition uri.Scheme == "spiffe"
(which indicates the user uses SPIFFE ID).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear if a valid SPIFFE certificate can only have one URI SAN field of any scheme or it can have multiple URI SAN fields, but only one of them can have a scheme of spiffe
. If its the latter, then the current check does not do the correct thing. It its the former, then you can check at the top before the for loop.
39297f1
to
3f3c77c
Compare
3f3c77c
to
9a3cd59
Compare
@easwars Thanks for the review! I've updated the code based on your comments. Please let me know if you see any other issues. Thanks! |
@@ -57,7 +67,7 @@ func SPIFFEIDFromState(state tls.ConnectionState) *url.URL { | |||
return nil | |||
} | |||
// A valid SPIFFE certificate can only have exactly one URI SAN field. | |||
if len(state.PeerCertificates[0].URIs) > 1 { | |||
if len(cert.URIs) > 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear if a valid SPIFFE certificate can only have one URI SAN field of any scheme or it can have multiple URI SAN fields, but only one of them can have a scheme of spiffe
. If its the latter, then the current check does not do the correct thing. It its the former, then you can check at the top before the for loop.
According to SPIFFE standards, "An X.509 SVID MUST contain exactly one URI SAN".
outside of the for loop, users using non-SPIFFE ID cert might also get the warning log(e.g. when they have |
LGTM. |
@easwars Thank you so much for the review! |
…FEIDFromCert (#3929) * internal/credentials: fix a bug and add one more helper function
The main purpose of this PR is to fix an issue I found when working on some integration tests of advancedtls: when checking the SPIFFE ID of the real certificate, we shouldn't check
len(uri.RawPath) == 0
sinceRawPath
will always be empty.Path
is the right one to use. I also added a test to parse a real certificate with SPIFFE ID in it.Besides, I added another helper function
SPIFFEIDFromCert
that will do mostly the same asSPIFFEIDFromState
, except that it parses the certificate directly. This will be used by advancedtls to get the SPIFFE ID at the time doing custom authorization(at that time, we can get the certificate, but thetls.ConnectionState
is not ready yet).@easwars @dfawley
Feel free to let me know if you have any questions. Thank you so much!